Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(casting): @agoric/casting MVP #5487

Merged
merged 11 commits into from
Jun 8, 2022
Merged

feat(casting): @agoric/casting MVP #5487

merged 11 commits into from
Jun 8, 2022

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Jun 1, 2022 •
edited
Loading

refs: #5385

Description

This is a complete, but polling-based chain casting follower. Being primarily event-driven is a goal for a future PR.
The best description is in https://github.com/Agoric/agoric-sdk/tree/mfig-chain-streams/packages/casting/README.md

As further PRs in this area are merged, the intent is to update this package to take advantage of new features. Notably, the #5466 x/vcast module will enable full chain support of the abstractions in #5418.

Security Considerations

Documentation Considerations

Testing Considerations

@michaelfig michaelfig added the cosmic-swingset package: cosmic-swingset label Jun 1, 2022
@michaelfig michaelfig self-assigned this Jun 1, 2022
@michaelfig michaelfig changed the title feat(streams): implement @agoric/streams library for following on-chain data without transactions feat(streams): @agoric/streams for read-only following of on-chain data Jun 1, 2022
@michaelfig michaelfig mentioned this pull request Jun 1, 2022
Closed
@michaelfig michaelfig force-pushed the mfig-chain-streams branch 3 times, most recently from c065cd7 to 7b1b27e Compare June 2, 2022 04:05
@michaelfig michaelfig changed the title feat(streams): @agoric/streams for read-only following of on-chain data feat(chain-streams): @agoric/chain-streams MVP Jun 2, 2022
@michaelfig michaelfig force-pushed the mfig-chain-streams branch 2 times, most recently from cde3857 to 528e040 Compare June 3, 2022 04:26
@michaelfig michaelfig marked this pull request as ready for review June 3, 2022 04:29
@michaelfig michaelfig requested a review from turadg as a code owner June 5, 2022 02:46
@michaelfig michaelfig requested a review from dckc June 5, 2022 02:54
@turadg turadg removed their request for review June 5, 2022 14:22
kriskowal
kriskowal approved these changes Jun 7, 2022
View reviewed changes
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding naming, I think “mailbox”, “publication”, “rendezvous”, “drop”, or “feed” might be more appropriate. I would like to reserve “stream” to mean bidi async iterator generally, which is what you’re returning from utility methods off your stream.

packages/chain-streams/README.md Outdated Show resolved Hide resolved
packages/agoric-cli/src/stream.js Outdated Show resolved Hide resolved
packages/chain-streams/src/defaults.js Outdated Show resolved Hide resolved
packages/chain-streams/src/leader.js Outdated
let lastRespondingEndpointIndex = 0;

/** @type {import('./types.js').ChainLeader} */
const leader = Far('round robin leader', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you’re measuring latency and error rate, The Power of Two Random Choices

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to read a paper right now. Can you be more specific as to what you're recommending, so that I can at least capture it as a TODO?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round-robin will be fine. Two random produces a slightly better load distribution when some hosts are more or less responsive than others, and does so in constant time, without maintaining a logarithmic heap.

const i1 = Math.floor(Math.random() * options.length)
const i2 = Math.floor(i1 + 1 + Math.random() * (options.length - 1)) % options.length;
if (latencies[i1] < latencies[i2]) {
  return options[i1];
} else {
  return options[i2];
}

Latencies are initialized to Infinity and updated with the latency of the most recent request/response (before consuming the response body).

@michaelfig
Copy link
Member Author

Regarding naming, I think “mailbox”, “publication”, “rendezvous”, “drop”, or “feed” might be more appropriate. I would like to reserve “stream” to mean bidi async iterator generally, which is what you’re returning from utility methods off your stream.

Thanks for the feedback. I think I've settled on @agoric/casting.

@michaelfig michaelfig changed the title feat(chain-streams): @agoric/chain-streams MVP feat(casting): @agoric/casting MVP Jun 7, 2022
@michaelfig michaelfig force-pushed the mfig-chain-streams branch 4 times, most recently from 5ca9217 to 7749ac4 Compare June 7, 2022 15:10
kriskowal
kriskowal approved these changes Jun 7, 2022
View reviewed changes
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the name change.

@michaelfig michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Jun 7, 2022
@mergify mergify bot merged commit ed2a918 into master Jun 8, 2022
@mergify mergify bot deleted the mfig-chain-streams branch June 8, 2022 00:31
JimLarson
JimLarson approved these changes Jun 17, 2022
View reviewed changes
Copy link
Contributor

@JimLarson JimLarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last retro-review: golang code looks good to me.

dckc
dckc reviewed Jun 17, 2022
View reviewed changes
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops... I just found some pending comments. Not critical, I suppose.

golang/cosmos/x/swingset/keeper/keeper.go Outdated
@@ -302,6 +303,19 @@ func (k Keeper) GetKeys(ctx sdk.Context, path string) *types.Keys {
return &keys
}

func (k Keeper) SetStorageAndNotify(ctx sdk.Context, path, value string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have go test coverage tooling?

packages/chain-streams/node-fetch-shim.js Outdated
/* global globalThis */
import fetch from 'node-fetch';

globalThis.fetch = fetch;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ouch. Why add a powerful global?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm. I suppose this only words in compartments where globalThis isn't frozen; i.e. where OCap discipline isn't enforced.

packages/chain-streams/test/fake-rpc-server.js Outdated
Comment on lines 66 to 73
export const startFakeServer = (t, fakeValues) => {
const { log = console.log } = t;
const { serialize } = makeMarshal();
lastPort += 1;
const PORT = lastPort;
return new Promise(resolve => {
log('starting http server on port', PORT);
const app = express();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exporting a startFakeServer that closes over express doesn't follow OCap discipline. How about putting app on t.context? so the caller would do t.context.app = express() in test.before()?

packages/chain-streams/test/fake-rpc-server.js Outdated
],
]);

export const develop = async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this develop() thing used?

packages/chain-streams/src/defaults.js Outdated
* @param {number} ms
* @returns {Promise<void>}
*/
export const delay = ms => new Promise(resolve => setTimeout(resolve, ms));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No modules should export powerful objects (for example, objects that close over fs).
-- https://github.com/Agoric/agoric-sdk/wiki/OCap-Discipline

End's node-powers.js is a nice approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dckc dckc dckc left review comments

@kriskowal kriskowal kriskowal approved these changes

@JimLarson JimLarson JimLarson approved these changes

@gibson042 gibson042 Awaiting requested review from gibson042

Assignees

@michaelfig michaelfig

Labels
automerge:no-update (expert!) Automatically merge without updates cosmic-swingset package: cosmic-swingset
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michaelfig @kriskowal @dckc @JimLarson